-
Notifications
You must be signed in to change notification settings - Fork 158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed problem escaping string in JSON #347
Fixed problem escaping string in JSON #347
Conversation
@btm @tas50 @lamont-granquist Please review the change. |
evaled_json = eval(json) | ||
rescue StandardError => e | ||
puts e.message | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does calling eval
on the json do, and what would cause a failure that is not fatal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @phiggins, as our vault json contents string with an hash format, using eval converting to the hash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a scenario where eval
can fail and evaled_json
is still a valid value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As in vault we accept only hash format, there is less possibility of failing the eval method.
lib/chef/knife/mixin/helper.rb
Outdated
@@ -27,6 +27,7 @@ def set_mode(mode) | |||
def merge_values(json, file) | |||
values = {} | |||
values.merge!(values_from_file(file)) if file | |||
validate_json(json) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this validation be moved into values_from_json
, so that bad values in json files will get caught in values_from_file
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, will move it, thanks
lib/chef/knife/mixin/helper.rb
Outdated
return false if string =~ /[^[:print:]]/ | ||
true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return false if string =~ /[^[:print:]]/ | |
true | |
/[^[:print:]]/.match?(string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this can be reduced to one line and it is only used in one place, I think it's appropriate to inline this code instead of having a function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok will update it, thanks @phiggins
msg = "Value '#{value}' of key '#{key}' contains non-printable characters. Check that backslashes are escaped with another backslash (e.g. C:\\\\Windows) in double-quoted strings." | ||
raise ChefVault::Exceptions::InvalidValue, msg | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will fail unless all of the values of the json are strings. That is, { ids: [1,2,3] }
, or { id: 2 }
will cause this to fail. Will the json always be in that format, or should this code try to handle those cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes @phiggins, it will work only for string values. any way to handle other formats?
Some unit tests would be good to demonstrate what's being fixed in this PR. |
@sanga1794 you'll need to rebase this since the commit that came from the UI lacks a DCO sign off. |
57322d5
to
c578b72
Compare
Signed-off-by: sanga17 <sausekar@msystechnologies.com>
Signed-off-by: sanga17 <sausekar@msystechnologies.com>
…e changes Signed-off-by: sanga17 <sausekar@msystechnologies.com>
c578b72
to
cd450db
Compare
Hello, |
@tas50 and @phiggins : This should not have been merged and should be reverted or redone. Valid JSON-spec strings are now rejected if they have completely JSON-valid escape sequences in them like |
Signed-off-by: sanga17 sausekar@msystechnologies.com
Description
knife vault : Problem escaping string in JSON
Related Issue
fixed: #346
Types of changes
Checklist: